scope3 rtd: update default params and docs#14378
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f156696d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| moduleConfig.endpoint = moduleConfig.endpoint || DEFAULT_ENDPOINT; | ||
| moduleConfig.timeout = moduleConfig.timeout || DEFAULT_TIMEOUT; | ||
| moduleConfig.includeKey = moduleConfig.includeKey || 'scope3_include'; | ||
| moduleConfig.excludeKey = moduleConfig.excludeKey || 'scope3_exclude'; | ||
| moduleConfig.macroKey = moduleConfig.macroKey || 'scope3_macro'; | ||
| moduleConfig.includeKey = moduleConfig.includeKey || 'axei'; | ||
| moduleConfig.excludeKey = moduleConfig.excludeKey || 'axex'; | ||
| moduleConfig.macroKey = moduleConfig.macroKey || 'axem'; |
There was a problem hiding this comment.
Changing the default includeKey/excludeKey/macroKey values here means any publisher who relied on the previous defaults (i.e., did not explicitly set these params and already created GAM line items with the old key names) will suddenly stop matching targeting after this update. That’s a behavior regression for existing integrations; consider keeping the old defaults, supporting both sets of keys during a transition, or providing an explicit migration path to avoid breaking live line items.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We are purposely changing this wrong default
There was a problem hiding this comment.
this is a breaking change, we do not typically allow this in a minor version. Do you have a reason you think it is okay to break everyone, eg this is a bugfix and it actually fixes them?
Pull Request Test Coverage Report for Build 21406962611Details
💛 - Coveralls |
patmmccann
left a comment
patmmccann
left a comment
There was a problem hiding this comment.
no breaking changes in minor versions
patmmccann
changed the title
|
@gravelg happy to merge on the 11 branch; does that work for you guys? |
|
Hey @patmmccann , sorry for the delay in response. For context, we only have two clients using this module right now and they are both setting the "new" keys in their config manually. While this is technically a breaking change, the current default configuration will simply not work with how we tell people to do the setup in GAM. |
|
fair enough, i will merge given only two clients and both of them aware |
|
docs change: prebid/prebid.github.io#6409 |
5 participants
Type of change
Bugfix
Feature
New bidder adapter
Updated bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Updated default params to match docs in code and markdown
Other information